-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Bugifx update all package repository fields #15477
Conversation
bootstrap is failing because the check-versions script is failing
It appears there are |
Those are caused by changes you did to |
Ok, I've dropped the commit that updated the package json |
Thank you! I'm wondering why the following syntax is not correct?
Official npm docs say it's okay https://docs.npmjs.com/files/package.json#repository |
Yes that would also be fine, I chose the style that most commonly already exists in this repo. I don't think it makes any significant difference. |
Made a tweak to the script to not insert repository as the last key in package.json which makes merge conflicts more likely, and rebased on the master branch. |
npm has support for monorepos now so I would suggest moving to this syntax
Sorry for the back and forth |
I tried that syntax and it doesn't work for the npm page link to GitHub. Until it does, I would recommend the current format. See my comment here: ea1d7d1#diff-5f64c3c255be83cf700e83e922506fa3R22 |
Thank you! I didn't go through the code yet 🙊. PR looks great! I'm loving the node script so we can change all our packages at any time 🤗 |
So I've noticed that most of the change logs are broken. I think this is due to Lerna prefixing urls with the "repository" url. This means "tree/master/packages/..." get's added to every link to issues, prs, commits, etc...and breaks the url. I'm guessing the new format (#15477 (comment)) mentioned by @wardpeet would fix this, but I get it may have other drawbacks. |
Okay, did some more reading, I now have some thoughts: @Graham42 How does the monorepo syntax not work for the nom page link? (my assumption is you mean it doesn't link you to the sub-directory) The more I think about this the repository link should be the root...and not include the folder path. NPM makes it clear that the 'repository" field is not for human consumption but for computers.
The "homepage" field already exists in the
Let me know if you have any questions. The full docs are here: https://docs.npmjs.com/files/package.json |
Correct, I was meaning the sub-directory. I missed the |
Also run as a check during linting to prevent future drift.
This commit can be recreated with "npm run check-repo-fields -- --fix"
Until these are included in the package.json workspaces field, the update-repo-fields script wont cover them.
Updated script to use the monorepo format, and to also update the After having to update this PR multiple times I've realize that the repo would easily drift after this fix so I've refactored the script a bit to work as a check that runs as part of the circle ci lint stage. Not sure if this might also fix #12755 now? |
Yes, it would close #12755 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge and see what it does 😄
Thanks for fixing! 🏅
Holy buckets, @Graham42 — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
Description
Looking at some gatsby packages on npm they are missing any link to the github repo source so it was difficult to find where the source code was.
This PR fixes all the
repository
fields for packages and add a script to be able to do this easily again in the future. Or if we want to make a sweeping change can update the script instead of manually updating each package.Something I'm not sure about: I had to update the lerna config to include the
themes
directory. I think this should be ok but not sure what else this might affect.Related Issues
Related: #12755